-
Notifications
You must be signed in to change notification settings - Fork 150
Implement mean min max for impact forecast and hazard forecast #1187
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…saving impact matrix
…/CLIMADA-project/climada_python into impactCalc_return_impactForecast
…/CLIMADA-project/climada_python into impactCalc_return_impactForecast
|
Probably you need to merge the forecast branch into this one. There are changes in the impact calc that do not belong here I think. |
climada/engine/impact_forecast.py
Outdated
|
|
||
| @property | ||
| def at_event(self): | ||
| LOGGER.warning("at_event for forecasts is not yet implemented.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is not true. At event is defined for the forecast.
| red_frequency = np.array([0]) | ||
| return red_event_id, red_event_name, red_date, red_frequency | ||
|
|
||
| def min(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the goal not to reduce along a specified dimension? I am no sure to see the value of getting the minimum impact accross all exposure points, all lead times, and all ensemble members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should reduce along the event dimension. Then we can compute the statistics over the member and/or lead time with the appropriate use of the select function. Does that makes sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me this method is exposed to the user, so it should directly give back a useful/meaningful result. So I would make the exposed method directly clear. Either over members, or over lead times (or both if the user for some reason wants it). Or make it private if the user is not supposed to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I would just make the complete reduction methods above private then.
Changes proposed in this PR:
Missing:
This PR fixes #1160
PR Author Checklist
develop)PR Reviewer Checklist